Fix build, tests, and clean up int types #62
Conversation
Elaborate fix for type chars showing up incorrectly on Windows for int32, int64 types. Also build fixes to work on local machine and remove some warnings.
|
(Sorry this snowballed into a larger change and I didn't break it up. If this has any chance of getting merged and you'd prefer it in smaller PRs just let me know.) |
|
Hey Alec, thanks a lot for this change! I agree with your points about types and this is a much cleaner API. I'm a bit busy this week but I'll review this and try to get it merged asap. I need to update point-cloud-utiles and an internal library at NVIDIA to match your changes. This should give some extra test coverage. Also, I have an open PR to integrate the very small change (added a public method to a bumpy class) from my fork, so we should soon be able to depend on mainline pybind. |
|
Sounds good. FWIW libigl-python-bindings main is currently ✅ building and ✅ passing tests with this branch. |
There was a problem hiding this comment.
There's one broken test that checks if you can pass in a None. This actually isn't supported behavior because it turns out to be quite hard to handle this case and there's an easier way to do it.
If you could comment out that line and make sure the tests pass, I'm ready to merge.
| @@ -104,20 +104,26 @@ def test_passing_0d_arrays(self): | |||
| def test_passing_0d_arrays_1(self): | |||
| dim = np.random.randint(5) | |||
| # (np.zeros([0, dim]), np.zeros([dim, 0]), np.zeros([0]), np.zeros([0, 0])): | |||
| dim = 2 | |||
There was a problem hiding this comment.
maybe separate out a dim=1 case
| self.assertEqual(len(a), 0) | ||
| # Alec: I'm not sure if it's expected behavior in numpy or a bug in | ||
| # numpyegien, but if dim == 1 then retp.shape = (0,) rather than (0,1) | ||
| if dim == 1: |
There was a problem hiding this comment.
NumpyEigen squeezes 1 dimensions by default when returning values, but there's an argument to npe::move (maybe it's hidden -- I don't recall atm) that changes this behavior.
| // | ||
| // But in the end is this an elaborate way to change dtype().type() 'Q','q' -> 'L','l' ? | ||
| // | ||
| // Yes, but I guess the point is that these checks are what the code should be doing instead of using the chars to begin with. |
There was a problem hiding this comment.
These characters are unique identifiers for the internal numpy types (https://numpy.org/doc/stable/reference/generated/numpy.dtype.char.html#numpy.dtype.char).
If I remember correctly, we need a way of translating from the pybind array type into an identifier, and also being able to return something in the case where the array is something weird (like Object) which will throw an error.
I suppose you're right though, this could all be inlined into the generated code.
|
Found a small bug that fixes the failing test. Merging your PR and will send a fix. |
dense_longlongmean the same thing on linux and windows? #59 by replacing non-standardint,longandlong longin C++ code withint32andint64and (omitted) and on the python/codegen side connectsintc/int32andint/int64appropriatelySince 128 integers are not supported in a standard way in C++ or numpy, it doesn't make sense to me to try to support them. (I didn't purge the complex256 related code, but along these lines it should also go).
When writing bindings, code will need to change according to:
npe_arg(x, dense_int, dense_long, dense_longlong)npe_arg(X, dense_int32, dense_int64)I don't understand the remaining failing test. I'm not sure what the correct behavior is supposed to be and I can't make sense of the macros defining the npe_default_arg
An upshot of this, is that a bunch of the
#ifdef WINstuff can be dropped. By insisting on explicitly declaring the int sizes, we get the benefits of usingstd::.While it would have been easier to just keep a
transform_type_charfor windows to filter'Q','q'into'L','l', the change I made usesisinstancechecks to truly verify that the type is the correct it. Ideally the chars could be eliminated entirely for compile-time type checks but I leave that for another day.